feat: Add Button.disabled can be a tooltip #202

stephenh · 2021-08-04T15:01:50Z

This is a) a blatant copy/paste from one of @DeanGilewicz 's WIP PRs + b) a long-standing need to be able to s/BP Button/Beam Button in BP, because we have quite a few places that use current BP Button's disabledReason prop.

chr1sjf0x

🍣

chr1sjf0x · 2021-08-04T15:53:40Z

src/interfaces.ts

  disabled?: boolean;
+  /** If set, will show a tooltip about why the button is disabled. */
+  disabledReason?: string;


FWIW: I had been musing with @KoltonG about whether it would be possible/good to have semi-direct integration with potential operations. Like:

Suggested change

disabled?: boolean;

/** If set, will show a tooltip about why the button is disabled. */

disabledReason?: string;

/** If a potential operation is provided, will show a tooltip about why the button is disabled when the operation is not allowed. */

disabled?: boolean | PotentialOperation;

I think whether this is a good idea is questionable, since it would directly tie the tooltip content to the potential operation disabled reasons... when in most cases you'd probably want some additional processing of those reasons 🤔

🤷

Hrm... yeah, that would be pretty cute, would be a easy if the reasons were just strings:

type PotentialOperation2 { allowed: Boolean! disabledReasons: [Reason!]! } type Reason { message: String! entity: HasBlueprintUrl } interface HasBlueprintUrl { blueprintUrl: Url! }

But since they're this complex Reason type, with pretty bespoke Blueprint-y things in it, yeah I'm not sure (as you already mentioned) we'd want to leak that directly into Beam...

Maybe if it was:

disabled: boolean | ReactNode

Where if disabled is ever "not a boolean" / i.e. a ReactNode (which could be just a string, or a complex structure) it knows that means "put the react node in the tooltip".

And then we had an adapter like:

<Button disabled=renderPotentialOperation(query.canDoFoo) />

Where the helper method would do app-specific rendering of the PotentialOperation2 instead "just some react nodes to put into the tooltip"...

@chr1sjf0x / @KoltonG pushed up another version that does:

disabled: boolean | ReactNode

Wdyt?

Looks awesome!

KoltonG

Nice! Will be nice to have this built in!

KoltonG · 2021-08-04T17:52:45Z

src/components/Button.stories.tsx

@@ -130,3 +130,7 @@ export function Buttons(args: ButtonProps) {
    </div>
  );
 }
+
+export function ButtonWithTooltip() {
+  return <Button disabled={true} disabledReason="You cannot currently perform this operation." label="Upload" />;


Thoughts: On having the disabled prop take in a boolean or a string for the message? Just thinking that disabled and disabledReason would always be in sync.

KoltonG · 2021-08-04T19:40:08Z

src/components/Tooltip.stories.tsx

+
+export function TooltipDisabled() {
+  return (
+    <Tooltip title="Tooltip Info" disabled={true}>


Interesting! I never knew a Tooltip could be disabled!

KoltonG · 2021-08-04T19:40:31Z

src/components/Tooltip.tsx

@@ -10,27 +10,30 @@ import { Css } from "src/Css";

 interface TooltipProps {
  /** The content that shows up when hovered */
-  title: string;
+  title: ReactNode;


Thank you! @kbesingeryeh the fix got in!

KoltonG · 2021-08-04T19:41:34Z

src/components/Tooltip.tsx

    state,
    triggerRef,
  );
  const { tooltipProps } = useTooltip(_tooltipProps, state);

  return (
    <>
-      {React.cloneElement(children, { ref: triggerRef, ...triggerProps })}
+      <span ref={triggerRef} {...triggerProps}>


Question: Any advantage to adding a wrapping span?

I'm pretty sure cloneElement ignores the ref prop (both ref and key are ignored, and the children's original ref/key are still used). I'm not really sure if this fixes anything per se, I'd just noticed that "cloneElement doesn't support ref" while doing other things.

It seems that cloneElement supports refs otherwise the Tooltip wouldn't work (maybe its ignored if it is given) 😢 Are you facing a case that you need to pass a ref to the Tooltip component (that might be why, since it's being overwritten)?

Here's an old conversation on the subject facebook/react#8873 (comment)

@oagboola and I approached it this way to reduce the wrapping elements needed for the Tooltip and add the Popper ref to the child element (assuming that the child would not have a ref of its own). All good either way!

I was just looking at:

https://reactjs.org/docs/react-api.html#cloneelement

"Clone and return a new React element using element as the starting point. The resulting element will have the original element’s props with the new props merged in shallowly. New children will replace existing children. key and ref from the original element will be preserved."

Yeah, it's that preserved behaviour I think is what is special - like if theres are no overrides then it will keep it the original ref. I'll CodeSandbox this for fun!

Here's a CodeSandbox showing that the children's ref is overridden by the clone elements ref! Still a very interesting result!

https://codesandbox.io/s/reactcloneelement-ref-override-gyyhm

Oh I see, it "preserves it" as in "it won't drop the existing one", but you're allowed to override it...playing devils advocate, if the caller did want to use the ref, i.e. the parentRef in your example, this approach drops it?

I'll put back though, given it's not broken as I originally though.

Also makes me think maybe we should have a few snapshot tests to show all of the aria-* stuff getting wired together. Between this change, and others, I'm not really sure when aria attributes are/are not getting hooked up, and that isn't covered by storybook.

KoltonG · 2021-08-04T19:41:44Z

src/interfaces.ts

+   *
+   * If a ReactNode, it's treated as a "disabled reason" that's shown in a tooltip.
+   */
+  disabled?: boolean | ReactNode;


## [2.37.0](v2.36.0...v2.37.0) (2021-08-04) ### Features * Add Button.disabled can be a tooltip ([#202](#202)) ([84b4641](84b4641))

homebound-team-bot · 2021-08-04T21:07:49Z

🎉 This PR is included in version 2.37.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: Add Button.disabledReason.

f080e39

stephenh requested review from bdow, chr1sjf0x, DeanGilewicz, KoltonG, ksk5280 and lucyconklin as code owners August 4, 2021 15:01

chr1sjf0x approved these changes Aug 4, 2021

View reviewed changes

KoltonG approved these changes Aug 4, 2021

View reviewed changes

stephenh added 2 commits August 4, 2021 13:40

Combine disabledReason into reason.

7fef0bb

Show the disabled can be a ReactNode.

c3de1e2

KoltonG reviewed Aug 4, 2021

View reviewed changes

stephenh added 2 commits August 4, 2021 14:46

Add support to IconButton.

039b39a

Fix ReactNode error in ButtonGroup.

40f4215

stephenh changed the title ~~feat: Add Button.disabledReason.~~ feat: Add Button.disabled can be a tooltip Aug 4, 2021

Go back to cloneElement.

48ac193

stephenh merged commit 84b4641 into main Aug 4, 2021

stephenh deleted the add-button-disabled-reason branch August 4, 2021 21:05

homebound-team-bot pushed a commit that referenced this pull request Aug 4, 2021

chore(release): 2.37.0 [skip ci]

a9998e8

## [2.37.0](v2.36.0...v2.37.0) (2021-08-04) ### Features * Add Button.disabled can be a tooltip ([#202](#202)) ([84b4641](84b4641))

homebound-team-bot added the released label Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Button.disabled can be a tooltip #202

feat: Add Button.disabled can be a tooltip #202

stephenh commented Aug 4, 2021

chr1sjf0x left a comment

chr1sjf0x Aug 4, 2021

stephenh Aug 4, 2021 •

edited

Loading

stephenh Aug 4, 2021

KoltonG Aug 4, 2021

chr1sjf0x Aug 4, 2021

KoltonG left a comment

KoltonG Aug 4, 2021

KoltonG Aug 4, 2021

KoltonG Aug 4, 2021

KoltonG Aug 4, 2021

stephenh Aug 4, 2021

KoltonG Aug 4, 2021

stephenh Aug 4, 2021

KoltonG Aug 4, 2021

KoltonG Aug 4, 2021

stephenh Aug 4, 2021

stephenh Aug 4, 2021

KoltonG Aug 4, 2021

homebound-team-bot commented Aug 4, 2021

feat: Add Button.disabled can be a tooltip #202

feat: Add Button.disabled can be a tooltip #202

Conversation

stephenh commented Aug 4, 2021

chr1sjf0x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoltonG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homebound-team-bot commented Aug 4, 2021

stephenh Aug 4, 2021 •

edited

Loading